feat: Add SEP-1034 elicitation schema default values#908
feat: Add SEP-1034 elicitation schema default values#908sainathreddyb wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
utafrali
left a comment
There was a problem hiding this comment.
The core logic is sound and test coverage is thorough, but there is a correctness bug: mutating the content map in-place silently fails when the user returns an unmodifiable map, with the exception swallowed at DEBUG level. There is also a design concern around applyDefaults leaking into the serialized MCP capabilities sent to the server, which risks breaking interoperability with strict servers.
| try { | ||
| applyElicitationDefaults(request.requestedSchema(), result.content()); | ||
| } | ||
| catch (Exception e) { |
There was a problem hiding this comment.
The catch (Exception e) silently swallows an UnsupportedOperationException if the user returns an unmodifiable map (e.g., Map.of(...) or Collections.unmodifiableMap(...)). Defaults are silently not applied, with only a DEBUG log. This is a real correctness hazard because nothing in the API contract prevents users from returning an unmodifiable content map.
The safer approach is to build a new merged map and return a new ElicitResult rather than mutating in-place:
Map<String, Object> merged = new HashMap<>(result.content());
applyElicitationDefaults(request.requestedSchema(), merged);
return new ElicitResult(result.action(), merged);This also removes the need for the try/catch entirely.
| */ | ||
| public Builder elicitation(boolean form, boolean url, boolean applyDefaults) { | ||
| this.elicitation = new Elicitation(form ? new Elicitation.Form(applyDefaults) : null, | ||
| url ? new Elicitation.Url() : null); |
There was a problem hiding this comment.
applyDefaults is a client-side SDK behavior flag, but because it lives inside ClientCapabilities.Elicitation.Form, it gets serialized and sent to the server in the initialize request. A strict MCP server that validates capabilities against the protocol schema will receive an unknown field it never declared. This could break interoperability.
Consider keeping applyDefaults purely in the SDK layer (e.g., a separate McpClientOptions / builder flag) rather than embedding it in the protocol capabilities record. Alternatively, document explicitly that this is an SDK extension field and that servers must tolerate unknown capability fields.
There was a problem hiding this comment.
Good catch on the interoperability concern.
This follows the TypeScript SDK's design — applyDefaults is defined inside FormElicitationCapabilitySchema and gets serialized to the server during initialization:
https://github.com/modelcontextprotocol/typescript-sdk/blob/main/packages/core/src/types/schemas.ts#L312
The Java SDK already uses @JsonIgnoreProperties(ignoreUnknown = true) on capability records, so servers built with this SDK will tolerate it. I've added Javadoc documenting this as an SDK-level behavior flag, consistent with the TypeScript reference implementation.
That said, if the maintainers prefer keeping it outside the protocol record (separate builder flag), happy to refactor.
| */ | ||
| public Builder elicitation(boolean form, boolean url, boolean applyDefaults) { | ||
| this.elicitation = new Elicitation(form ? new Elicitation.Form(applyDefaults) : null, | ||
| url ? new Elicitation.Url() : null); |
There was a problem hiding this comment.
The new builder overload elicitation(boolean form, boolean url, boolean applyDefaults) silently drops applyDefaults when form == false. A caller writing elicitation(false, true, true) gets a capability object where applyDefaults is never set, with no warning. Since shouldApplyElicitationDefaults() then returns false, defaults are never applied.
At minimum, add a guard:
if (!form && applyDefaults) {
throw new IllegalArgumentException("applyDefaults requires form to be true");
}Or document the behavior clearly in the Javadoc.
There was a problem hiding this comment.
Good catch. Added an IllegalArgumentException guard — elicitation(false, true, true) now throws with "applyDefaults requires form to be true".
| .elicitation(elicitationHandler) | ||
| .build()) { | ||
|
|
||
| InitializeResult initResult = mcpClient.initialize(); |
There was a problem hiding this comment.
The integration test only exercises the HashMap path where mutation succeeds. The real-world failure mode (unmodifiable content map from Map.of()) has no test coverage. Consider adding a second variant:
return new McpSchema.ElicitResult(McpSchema.ElicitResult.Action.ACCEPT, Map.of());This would surface the UnsupportedOperationException bug described above, confirming the fix is needed.
|
|
||
| assertThat(capabilities.elicitation()).isNotNull(); | ||
| assertThat(capabilities.elicitation().form()).isNotNull(); | ||
| assertThat(capabilities.elicitation().form().applyDefaults()).isTrue(); |
There was a problem hiding this comment.
assertThat(json).contains("true") is too broad — it will pass as long as any boolean true appears anywhere in the serialized JSON, not specifically as the value of applyDefaults. Use a more targeted assertion:
assertThat(json).contains("\"applyDefaults\":true");or use assertThatJson(json).node("elicitation.form.applyDefaults").isEqualTo(true).
| @@ -0,0 +1,151 @@ | |||
| /* | |||
| * Copyright 2024-2024 the original author or authors. | |||
There was a problem hiding this comment.
Copyright year says 2024-2024. It should be 2024-2025 to match the other files in the project.
Add support for the applyDefaults capability in elicitation form mode, allowing clients to indicate they will apply schema-defined default values to elicitation results before returning them to the server. Changes: - Add applyDefaults field to Elicitation.Form capability record - Add elicitation(boolean, boolean, boolean) builder overload - Implement default value application in McpAsyncClient - Add comprehensive tests for default value merging behavior - Add serialization round-trip tests for the new capability Ref modelcontextprotocol#908
eaf75e8 to
26398a4
Compare
Add client-side default application for elicitation schemas, matching the behavior specified in SEP-1034 and implemented in the TypeScript SDK.
Changes:
Closes #699
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context